Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

FIX: Prevent unnecessary cache writes (fixes #34) #58

Closed
wants to merge 1 commit into from
Closed

FIX: Prevent unnecessary cache writes (fixes #34) #58

wants to merge 1 commit into from

Conversation

kinglozzer
Copy link
Member

See #34.

CachedConfigCollection is the only ConfigCollection implementation that doesn’t implement MutableConfigCollection, so IMO it’s clearly not designed to be mutable.

The nested config collection could theoretically be mutable, but I think that further emphasises that we should remove __destruct() here because we have no idea if committing the resulting collection is desirable. If two different code paths modify the collection differently, which (if any) should be cached?

In framework when the collection is nested we don’t actually operate on CachedConfigCollection::$collection anyway, because we construct a new DeltaConfigCollection instead:

https://github.com/silverstripe/silverstripe-framework/blob/c8990e69495cbc41ab78cca767f264466eecf6cd/src/Core/Config/CoreConfigFactory.php#L52-L55

It looks to me like the only BC risk here is if someone is interacting with this package directly, and nesting a cached config collection, and expecting any changes on the resulting MutableConfigCollection to be automatically cached. IMO that’s acceptably low, but I suppose we could diff the entire config array - though I’m not sure if that would negate any performance benefits.

Unscientific benchmarks:

Before: 
ab -n 250 -c 10 http://mysite.test/
This is ApacheBench, Version 2.3 <$Revision: 1879490 $>
Copyright 1996 Adam Twiss, Zeus Technology Ltd, http://www.zeustech.net/
Licensed to The Apache Software Foundation, http://www.apache.org/

Benchmarking mysite.test (be patient)
Completed 100 requests
Completed 200 requests
Finished 250 requests


Server Software:        Apache/2.4.46
Server Hostname:        mysite.test
Server Port:            80

Document Path:          /
Document Length:        32700 bytes

Concurrency Level:      10
Time taken for tests:   21.361 seconds
Complete requests:      250
Failed requests:        0
Total transferred:      8342000 bytes
HTML transferred:       8175000 bytes
Requests per second:    11.70 [#/sec] (mean)
Time per request:       854.426 [ms] (mean)
Time per request:       85.443 [ms] (mean, across all concurrent requests)
Transfer rate:          381.38 [Kbytes/sec] received

Connection Times (ms)
              min  mean[+/-sd] median   max
Connect:        0    0   0.0      0       0
Processing:   322  824 180.5    715    1053
Waiting:      318  819 180.5    711    1049
Total:        322  824 180.5    715    1053

Percentage of the requests served within a certain time (ms)
  50%    715
  66%    997
  75%    998
  80%    999
  90%   1008
  95%   1016
  98%   1044
  99%   1052
 100%   1053 (longest request)
After: 
ab -n 250 -c 10 http://mysite.test/
This is ApacheBench, Version 2.3 <$Revision: 1879490 $>
Copyright 1996 Adam Twiss, Zeus Technology Ltd, http://www.zeustech.net/
Licensed to The Apache Software Foundation, http://www.apache.org/

Benchmarking mysite.test (be patient)
Completed 100 requests
Completed 200 requests
Finished 250 requests


Server Software:        Apache/2.4.46
Server Hostname:        mysite.test
Server Port:            80

Document Path:          /
Document Length:        32700 bytes

Concurrency Level:      10
Time taken for tests:   20.838 seconds
Complete requests:      250
Failed requests:        0
Total transferred:      8342000 bytes
HTML transferred:       8175000 bytes
Requests per second:    12.00 [#/sec] (mean)
Time per request:       833.517 [ms] (mean)
Time per request:       83.352 [ms] (mean, across all concurrent requests)
Transfer rate:          390.94 [Kbytes/sec] received

Connection Times (ms)
              min  mean[+/-sd] median   max
Connect:        0    0   0.1      0       0
Processing:   310  803 175.6    664    1028
Waiting:      308  801 175.6    662    1027
Total:        310  803 175.6    664    1029

Percentage of the requests served within a certain time (ms)
  50%    664
  66%    973
  75%    975
  80%    976
  90%    980
  95%    986
  98%    991
  99%   1008
 100%   1029 (longest request)

@kinglozzer
Copy link
Member Author

I’ve been investigating this a bit more today, and I’m not satisfied that this really improves the status quo. There may be a performance improvement, but it’s heavily dependent on I/O speed (as I found today benchmarking on a different machine), and the drawback of this change is that the “callCache” is always empty at the start of a request, so there’s actually a possible performance hit.

I still think #34 needs to be addressed, but I don’t think this change alone is the right approach

@kinglozzer kinglozzer closed this May 31, 2022
@kinglozzer kinglozzer deleted the apetite-for-__destruction branch May 31, 2022 16:35
@GuySartorelli GuySartorelli mentioned this pull request Jul 4, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
1 participant
@kinglozzer